Skip to content

feat(log): add stopChainOnFailure handler option - #10578

Open
Alexandros-Pallis wants to merge 1 commit into
codeigniter4:4.8from
Alexandros-Pallis:feature/change-logger-handler-interface-behavior
Open

Alexandros-Pallis wants to merge 1 commit into
codeigniter4:4.8from
Alexandros-Pallis:feature/change-logger-handler-interface-behavior

Conversation

@Alexandros-Pallis

@Alexandros-Pallis Alexandros-Pallis commented Sep 21, 2026 •

Copy link
Copy Markdown

Note: This PR originally changed HandlerInterface::handle() to return int with RESULT_CONTINUE /
RESULT_STOP constants. That approach was dropped after review in favour of a config option. Review comments the force-push refer to the original implementation.
Follow-up to #10560.
Logger::log() stops running handlers when one returns false from handle(). FileHandler and ErrorlogHandler
return false when a write fails, so one failing handler (e.g. FileHandler with bad permissions on the log directory) silently prevents every handler after it from logging. A configured ErrorlogHandler fallback never runs and the log
entry is lost.

This PR adds a per-handler option, stopChainOnFailure, for FileHandler and ErrorlogHandler, set in the handler
settings of Config\Logger:

  • true (default, also when the key is missing): a failed write returns false and stops the chain. Same as today. - false: a failed write returns true, so the handlers after it still run.

The option is read in BaseHandler, so custom handlers extending it can use it as well. Custom handlers can still return false to stop the chain on purpose.

There is no breaking change:

  • HandlerInterface::handle() still returns bool, and Logger::log() is not modified.
  • The default keeps the current behavior, so existing app/Config/Logger.php files work unchanged.
  • BaseHandler gains a new protected property, bool $stopChainOnFailure.

Project files:

  • app/Config/Logger.php: stopChainOnFailure added to the FileHandler block and to the commented ErrorlogHandler block. The @var type of $handlers now includes bool (same in MockLogger).

Tests:

  • LoggerTest: a FileHandler pointed at a missing directory followed by TestHandler. With the default, the second handler does not log. With 'stopChainOnFailure' => false, it does.
  • FileHandlerTest and ErrorlogHandlerTest: default, explicit true, and false on a failed write. FileHandlerTest also covers a successful write with false.

User guide:

  • general/logging.rst: new section "Stopping the Handler Chain on Failure" with an example.
  • v4.8.0.rst: entry under Enhancements.
  • upgrade_480.rst: app/Config/Logger.php listed under the config content changes.

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value (without duplication)
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@carson-codeigniter4 carson-codeigniter4 Bot added 4.8 PRs that target the `4.8` branch. refactor Pull requests that refactor code labels Sep 21, 2026

@neznaika0 neznaika0 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks logical, but now we practically have no option to stop the handler - result is always returned to continue processing.

Perhaps, after these changes, stopping is not required at all, in which case, what’s the point of the new constants?

@michalsn michalsn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this behavior should be configurable via Config\Logger, and preferably the default behavior should remain the same as it is now.

@paulbalandan

Copy link
Copy Markdown
Member

Thanks for picking this up. Looking closer I think the interface change isn't what fixes the bug.

TLDR: A change to int return with only two states looks like just a renamed true/false return.

The fix is the built-in handlers no longer returning the "stop" signal on a write failure. That works with the existing bool: on 4.8, changing only FileHandler to return true when fopen() fails makes a failing FileHandler followed by another handler log correctly, and the Log tests still pass. With int, the built-ins return RESULT_CONTINUE on success and failure alike, so CONTINUE/STOP end up as a rename of true/false. That doesn't justify every custom handler failing at class load, which I think is @neznaika0's point as well.

Building on @michalsn's suggestion, I'd propose keeping bool and making it a per-handler option in the handler settings of Config\Logger, for example 'stopOnFailure' => true (the config name is debatable) for FileHandler and ErrorlogHandler:

  • true (the default) keeps today's behaviour: a failed write returns false and stops the chain.
  • false makes the handler return true on a failed write, so the handlers after it still run.

Handlers should fall back to true when the key is missing, so existing app/Config/Logger.php files keep working unchanged. Custom handlers can still return false to stop the chain on purpose. The key would need to be added to app/Config/Logger.php, described in general/logging.rst, and mentioned in the changelog.

For tests, please add LoggerTest cases for the reported scenario, a FileHandler pointed at a missing directory followed by TestHandler: one with 'stopOnFailure' => false asserting the second handler still logs, and one with the default asserting it doesn't, so today's behaviour stays pinned. The FileHandler and ErrorlogHandler tests should cover both values too.

A rebase onto 4.8 should clear the PHPStan failure.

@Alexandros-Pallis

Copy link
Copy Markdown
Author

I also agree that going with the config solution is the way to go. I will change the implementation to match what is described above.

One suggestion i have is the naming of the config, in my opinion 'stopChainOnFailure' is a bit more descriptive on the purpose of this option. Is it okay if i name it this way or keep 'stopOnFailure' as suggested initially?

Thanks everyone for the feedback!

A failed write in FileHandler or ErrorlogHandler returns false, which
stops the handler chain, so a fallback handler configured after it
never logs the message.

Setting 'stopChainOnFailure' => false in the handler config makes a
failed write return true, letting the remaining handlers run. Defaults
to true, so existing configs and HandlerInterface are unchanged.
@Alexandros-Pallis Alexandros-Pallis changed the title refactor(log): return int from HandlerInterface::handle() feat(log): add stopChainOnFailure handler option Oct 1, 2026
@carson-codeigniter4 carson-codeigniter4 Bot added needs template Opened issues not following the bug form template and removed refactor Pull requests that refactor code labels Oct 1, 2026
@carson-codeigniter4

Copy link
Copy Markdown

Hi there, @Alexandros-Pallis! 👋

It looks like this pull request does not follow our template:

Please update the description to follow the template. The needs template label will be removed automatically once it does.

@Alexandros-Pallis

Alexandros-Pallis commented Oct 1, 2026 •

Copy link
Copy Markdown
Author

Based on above discussion i have adjusted the direction of this PR and moved towards a config based solution.
Let me know if this is okay, thanks!

@Alexandros-Pallis
Alexandros-Pallis force-pushed the feature/change-logger-handler-interface-behavior branch from 80803b5 to d2606ef Compare October 1, 2026 18:23

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4.8 PRs that target the `4.8` branch. needs template Opened issues not following the bug form template

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants